Calendarプログラムの作成 by jepe-w · Pull Request #2 · jepe-w/ruby-practices

jepe-w · 2024-02-29T04:21:02Z

No description provided.

maedana · 2024-03-05T07:21:37Z

02.calendar/calendar.rb

+end
+
+result_calendar = WorkClendar.new
+result_calendar.calendar


細かいのですが、末尾は改行しましょう

maedana · 2024-03-05T07:21:57Z

02.calendar/calendar.rb

+
+class WorkClendar
+  def initialize()
+  opt = OptionParser.new


適切にindentしましょうー

maedana · 2024-03-05T07:28:46Z

02.calendar/calendar.rb

+  opt = OptionParser.new
+
+  opt.on('-y') {|v| v}
+  opt.on('-m') {|v| v}


何を参考にして書きました? これではブロックを利用する意味ないかなと思います。

こちらは当時使い方を理解し切れておらず公式のリファレンスをそのまま参考にした気がします。。
手元に社用PCがないので、明日修正します！

maedana · 2024-03-05T07:32:46Z

02.calendar/calendar.rb

+  opt.on('-m') {|v| v}
+
+  @request = opt.parse!(ARGV)
+  @request = ARGV


これではあまり効果的にクラスやインスタンス変数を使えてないように見えますね。なぜこのような設計にしたんでしょう?

WorkCalendarというクラス名ですが、Workってどこから出てきた名前だろう?

@requestという変数はどういう意図の命名だろう? 中身はyearとmonthの配列なのにrequest?

parse!は ARGVを破壊的に変更するし、ARGVはどこからでもアクセス出来るからそもそも変数にいれる意味あるのだろうか?

などが気になります。

全体的に変数の命名の仕方に問題がありそうですね。
こうみると確かに自分にしかわからないような変数になっているのがよくわかります😢
実務に入る前に直していけるようにします！以下、変数などの命名理由です。（少し前に行ったものなので記憶があやふやなところもあります）

Workはカレンダーの課題なので、カレンダーワークというイメージからWorkとつけました。😢
リクエストの意図は確か、日にちを指定できるという仕様があったからだったと思います。。
parse!については、初めて使うものだったということもあり、リファレンスあたりを参考に書いた気がします。。

Workはカレンダーの課題なので、カレンダーワークというイメージからWorkとつけました。😢

既に修正されたようですが、課題だからというのはクラス名の命名に盛り込むのはおかしいかなと思います。あくまでどんなクラスなのか?をベースに考えましょう。このへんは後半の課題でオブジェクト指向に関するものがあるのでそこで改めて学びましょう。

リクエストの意図は確か、日にちを指定できるという仕様があったからだったと思います。

なるほど。ただ、requestだけだと日にちが入ってるようには全く見えないので、もう少し工夫したほうがよかったかなと思います。

parse!については、初めて使うものだったということもあり、リファレンスあたりを参考に書いた気がします。

リファレンスを参照するのは大事なのでいいですね 👍 その上で書いてある内容を理解して使うようにするとなおよいと思います 😄

maedana · 2024-03-05T07:33:32Z

02.calendar/calendar.rb

+  @request = ARGV
+  end
+
+  def calendar


メソッドは原則動詞から始まる命名にするのがセオリーです。このメソッドは何をする役割でしょう? その部分を命名に盛り込みたいですね。

maedana · 2024-03-05T07:46:05Z

02.calendar/calendar.rb

+
+    #日にちを取得
+    designated_date = Date.new(default_year.to_i, default_month.to_i, 1)
+    month_days = ((designated_date >> 1) - 1).day


月末の日を取得したいだけですよね。
https://bootcamp.fjord.jp/practices/194#-3
のプラクティスのヒントを確認しましょうー。もっと簡単に月末を取得する方法が載ってますよ。このコードはちょっとトリッキー過ぎて読みくいです。僕は初見で読めませんでした。

maedana · 2024-03-05T07:47:28Z

02.calendar/calendar.rb

+    #日にちを取得
+    designated_date = Date.new(default_year.to_i, default_month.to_i, 1)
+    month_days = ((designated_date >> 1) - 1).day
+    date = designated_date.wday


dateという命名からはDate型のインスタンスを想像するのが普通かなと思います。実際はそうではないので命名変えたいですね。

maedana · 2024-03-05T07:48:22Z

02.calendar/calendar.rb

+    end
+
+    #余白を含めた日にちを一週間ごとに分割
+    new_days = days.each_slice(7).to_a


なぜnewなのだろうって思いました。既にdays使ってるし...ぐらいの理由だとしたらもっとちゃんとした命名を練りたいところです。

maedana · 2024-03-05T07:49:13Z

02.calendar/calendar.rb

+    end
+
+    #日にちを取得
+    designated_date = Date.new(default_year.to_i, default_month.to_i, 1)


これはわかりやすい命名なのですが、初日であるということも命名に盛り込まれるとなおわかりやすいんじゃないかなと思いました。いかがでしょうか。

ありがとうございます！
参考にさせていただきます！

maedana · 2024-03-05T07:50:00Z

02.calendar/calendar.rb

+
+    #一週間ごとに分けた配列をそれぞれくっつけて曜日と一緒に出力。
+    puts ("日 月 火 水 木 金 土")
+    new_days = new_days.map{|item| puts item.join(" ")}


変数に入れ直しているのはなぜでしょう?

なぜmapにしているのでしょう?

maedana · 2024-03-06T08:13:59Z

02.calendar/calendar.rb

 #!/usr/bin/env ruby
 require "date"
 require "optparse"
+require 'debug'


デバッグ用の記述は提出物からは消しましょう

maedana · 2024-03-06T08:14:47Z

02.calendar/calendar.rb

+require 'debug'

-class WorkClendar
+class Clendar


クラス名については違和感なくなりました。一方でインスタンス変数がゼロになったので、もはやクラス使う意味がないなと思います。したがってクラスなしにしましょうか。クラスについては後半の課題で出てくるのでそのときに改めて学ぶのがいいんじゃないかなと思います。

maedana · 2024-03-06T08:17:16Z

02.calendar/calendar.rb

-  @request = opt.parse!(ARGV)
-  @request = ARGV
+    opt.on('-y') 
+    opt.on('-m') 


ブロックなくなりましたが、前回の指摘の意図としてはブロックをうまく活用してほしいな、ということでした。

opt.parse!(ARGV)

した後のARGVの中身ってあまり扱いやすいものではない(ARVG[0]やARGV[1]のようなものってリーダブルじゃないですよね)と思いますので。もし難しいならば、
https://docs.ruby-lang.org/ja/latest/method/OptionParser/i/getopts.html
というのもあるのでこちらを使うほうがわかりやすいかなと思います。

maedana · 2024-03-06T08:18:25Z

02.calendar/calendar.rb

    else
-      default_month = @request[1]
+      target_month = ARGV[1]
    end


上で指摘しましたが、getopt使うようにするとこの辺の実装も変わってきそうです。

maedana · 2024-03-06T08:19:10Z

02.calendar/calendar.rb

-    month_days = ((designated_date >> 1) - 1).day
-    date = designated_date.wday
+
+    first_day_of_designated_date = Date.new(target_year.to_i, target_month.to_i, 1)


もはやシンプルにfirst_dayくらいで十分わかるんじゃないですかね。なお、dayとdateは意味が違ってきます。今回どちらがより適切かも考えてみてください。

maedana · 2024-03-06T08:20:15Z

02.calendar/calendar.rb

-    date = designated_date.wday
+
+    first_day_of_designated_date = Date.new(target_year.to_i, target_month.to_i, 1)
+    month_days = Date.new(target_year.to_i, target_month.to_i, -1).day


前回指摘しなかったのですが、複数形の命名は通常は配列だと思ってしまいます。この場合単なる数字なので複数形を使わずに適切な命名を考えてみてほしいです。

maedana · 2024-03-06T08:21:11Z

02.calendar/calendar.rb

+
+    first_day_of_designated_date = Date.new(target_year.to_i, target_month.to_i, 1)
+    month_days = Date.new(target_year.to_i, target_month.to_i, -1).day
+    get_day_of_week = first_day_of_designated_date.wday


get_day_of_weekですと動詞始まりなのでメソッド名のような命名に感じます。変数名は名詞や形容詞+名詞など、動詞を使わない命名をするのが自然だと思います。もう少し練ってみてください 🙏

maedana · 2024-03-06T08:25:17Z

02.calendar/calendar.rb

-      if num < 10
-        days.push(" #{num}")
+    month_days.times do |n|
+      n += 1


1始まりにしたいからこうしてるんですよね? うまく工夫すれば1はじまりでループする方法があると思うんです。+1せずに済むコードにしてみてほしいのですが難しいでしょうか。

maedana · 2024-03-06T08:25:58Z

02.calendar/calendar.rb

+    puts first_day_of_designated_date.strftime("%B %Y").center(20)
    puts ("日 月 火 水 木 金 土")
-    new_days = new_days.map{|item| puts item.join(" ")}
+    week.each{|day| puts day.join(" ")}


凄く細かいですが、 { }の前後は空白あけましょう

maedana · 2024-03-06T08:27:35Z

動作確認してみると、-yだけ、-mだけ指定したときにエラーが発生します。-yだけのときは月は当月、-mだけのときは年は今年となるような動きにしましょう。

maedana · 2024-03-13T05:54:59Z

02.calendar/calendar.rb

+  opt.on('-y Integer') { 
+    |y| option[:y] = y
+    target_year = option[:y]
+  }


ちょっと書き方がおかしいかなーと思います。

ブロック変数のところは同じ行に書きましょう

ブロック内が複数行のときは { }でなくて、do endを使いましょう

NG

[1,2,3].each { |i| i }

OK

[1,2,3].each { |i| i } [1,2,3].each do |i| # 通常同じ記述を繰り返しませんが、複数行の表現のための便宜上です。 puts i puts i end

option[:y] = y target_year = option[:y]

はoptions[:y]に一旦代入する意図がよくわからなかったです。直接target_yearに入れない理由がなにかあるのでしょうか。

maedana · 2024-03-13T05:56:59Z

02.calendar/calendar.rb

+  opt.on('-y Integer') { 
+    |y| option[:y] = y
+    target_year = option[:y]
+  }


option[:y] = y target_year = option[:y]

はoptions[:y]に一旦代入する意図がよくわからなかったです。直接target_yearに入れない理由がなにかあるのでしょうか。

maedana · 2024-03-13T06:00:58Z

02.calendar/calendar.rb

-    if ARGV == []
-      target_month = Date.today.mon
+  first_day = Date.new(target_year.to_i, target_month.to_i, 1)
+  month_day = Date.new(target_year.to_i, target_month.to_i, -1).day


月末の日付のことを英語でmonth dayと表現するのでしたっけ? もしそうならいいのですが、そうでないならおかしいのかな思います。いかがでしょう? いい表現が思いつかないのであれば、first_dayとの対比で

last_day = month_day = Date.new(target_year.to_i, target_month.to_i, -1)

とした上で、month_dayの利用箇所をlast_dayを使ったもので書き換えてもよいかな、と思います。

今更ですが僕の記述が変ですね

last_day = Date.new(target_year.to_i, target_month.to_i, -1)

のつもりでした。

maedana · 2024-03-13T06:01:25Z

02.calendar/calendar.rb

+    days.push("  ")
+  end
+  #その月の日数分daysに日にちを追加
+  1.upto(month_day) do |d|


+1しなくてよくなりましたね 👍

maedana · 2024-03-13T06:02:44Z

02.calendar/calendar.rb

+  month_day = Date.new(target_year.to_i, target_month.to_i, -1).day
+  day_of_week = first_day.wday
+
+  #曜日ぶんから文字を挿入（初日のスタート位置（曜日）をとるため）


もしかしたら既に指摘しているかもですが、説明的なコメントは可能な限り避けましょう。

参考
https://qiita.com/jnchito/items/f0d90af4ed44b7484103

とはいえ慣れないうちは難しいかもしれないですし、無理になくさなくても大丈夫です。

maedana · 2024-03-16T08:53:20Z

02.calendar/calendar.rb

+  opt.parse!(ARGV)
+
+  first_day = Date.new(target_year.to_i, target_month.to_i, 1)
+  last_day = Date.new(target_year.to_i, target_month.to_i, -1).day


first_day = Date.new(target_year.to_i, target_month.to_i, 1) last_day = Date.new(target_year.to_i, target_month.to_i, -1).day

で、first_dayはDate型クラスなのに対して、last_dayは単なるIntegerの値です。これでは対比にならないですね。.dayの呼び出しはlast_dayの参照箇所(一箇所しかない)でやればいいのでは、と思います。

あと、_dayは_dateのほうがもしかしたらわかりやすいかもしれません。Date型だからです。

参考
https://bootcamp.fjord.jp/pages/readable-variable-name

maedana · 2024-03-16T09:00:15Z

02.calendar/calendar.rb

+  end
+
+  1.upto(last_day) do |d|
+    single_digit_date = d < 10


条件判定を変数にしたのですね。この命名ですとbooleanのイメージが湧きにくいかもしれません。こうしたいのであれば、is_single_digit_dateのようなものであるほうがbooleanと伝わりやすいかもしれません。booleanとしてどんな命名がいいかはプログラミング一般の話として検索すると出てくると思います。よくあるのはis_xxx, has_xxx, can_xxx,や、末尾をxxxableのようなものにするなどでしょうか。

Rubyの場合、メソッドであれば?が使えるのでbooleanの値を返すメソッドなら末尾に?をつけるのが一般的ですね。

そしてそもそもを言うと、これくらいならばd < 10で十分読みとけるので命名に悩むくらいならそのまま書いてもいいかな、とは思います。

maedana · 2024-03-18T06:20:37Z

02.calendar/calendar.rb

+  opt.parse!(ARGV)
+
+  first_date = Date.new(target_year.to_i, target_month.to_i, 1)
+  last_day = Date.new(target_year.to_i, target_month.to_i, -1)


first_dateを直したのなら、last_dayも直しましょう。くどいようですが、これでは対比にならない命名ですし、どちらもDate型ですから。

jepe-w added 2 commits February 28, 2024 14:26

Assignment submission Calendar

6d4b5d3

インデントの修正

7a83d14

maedana reviewed Mar 5, 2024

View reviewed changes

レビュー指摘箇所の改善

7660057

maedana reviewed Mar 6, 2024

View reviewed changes

レビューでの指摘箇所改善　v2

adb31b6

maedana reviewed Mar 13, 2024

View reviewed changes

指摘箇所の改善　v3

996c948

maedana reviewed Mar 16, 2024

View reviewed changes

レビュー指摘箇所の改善v3

39501b7

maedana reviewed Mar 18, 2024

View reviewed changes

変数改善

911d91a

maedana approved these changes Mar 19, 2024

View reviewed changes

Conversation

jepe-w commented Feb 29, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chantakens Mar 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maedana commented Mar 6, 2024

Uh oh!

maedana Mar 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

chantakens Mar 5, 2024 •

edited

Loading

maedana Mar 13, 2024 •

edited

Loading